Vizzly - Visual Test Results

_{feat/plugin-git-api · 8f920eca}

Code Review

I've reviewed this PR and overall it's a well-designed improvement to the plugin API. The change from fragile dynamic imports to a proper API is the right approach. Here's my detailed feedback:

✅ Strengths

1. Excellent API Design

• Clean, stable interface in src/plugin-api.js:35-62 that properly abstracts git detection
• Good encapsulation using Object.freeze() to prevent modifications
• Clear JSDoc documentation explaining purpose and return values
• Backwards-compatible fallback strategy works well

2. Correct CI Environment Handling

• Properly delegates to existing battle-tested utils/git.js and utils/ci-env.js
• Handles GitHub Actions PR merge commit issue correctly (reading from event payload)
• Supports wide range of CI providers through existing infrastructure

3. Clean Migration Path

• Both plugins (clients/storybook/src/index.js:115-135 and clients/static-site/src/index.js:169-191) handle old and new CLI versions gracefully
• Fallback to environment variables ensures compatibility
• Peer dependency bump clearly signals version requirement

4. Code Quality

• Removes fragile dynamic import hackery (good riddance to those try/catch chains!)
• Simplifies plugin code significantly (-15 lines in storybook, -13 in static-site)
• More readable and maintainable

⚠️ Issues & Concerns

1. Missing Test Coverage (Critical)

The new services.git.detect() API has zero test coverage. This is a public API that plugins depend on, and it should have comprehensive tests covering:

• ✅ Returns correct git info in normal git repo
• ✅ Handles GitHub Actions PR context (merge commit SHA vs head SHA)
• ✅ Handles missing git repo gracefully
• ✅ Respects environment variable overrides (VIZZLY_*)
• ✅ Correctly formats buildName with and without prefix
• ✅ Returns proper structure { branch, commit, message, prNumber, buildName }

Recommendation: Add tests/unit/plugin-api.spec.js with comprehensive coverage before merging.

2. Inconsistent Error Handling

src/plugin-api.js:45-61 calls multiple async git functions but doesn't handle errors. What happens if:

• Not in a git repo?
• Git command fails?
• CI environment vars are malformed?

Looking at utils/git.js, the underlying functions return null on failure, but the plugin API doesn't document this behavior.

Recommendation: Either:

• Document that null values are possible in the return object
• Add try/catch and return safe defaults explicitly
• Consider adding a success: boolean field to the return object

3. Silent Peer Dependency Issues

The fallback in plugins (clients/storybook/src/index.js:126-135) silently uses environment variables when services.git?.detect is missing, but there's no warning to users that they should upgrade the CLI.

Recommendation: Add a one-time warning when falling back:

if (services.git?.detect) {
  // Use CLI's git detection
} else {
  logger.warn('⚠️  Using old CLI version. Upgrade to @vizzly-testing/cli@>=0.25.0 for improved git detection.');
  // Fallback...
}

4. Type Safety Concerns

The PR description mentions "peer dep warning" but I don't see where this warning would be shown. The peerDependencies in package.json will cause npm/yarn to warn during install, but:

• Not all package managers enforce peer deps
• Users might miss the warning
• Runtime detection would be more reliable

5. Potential Race Condition

src/plugin-api.js:46-50 uses Promise.all() which is good, but detectPullRequestNumber() at line 51 is synchronous (only reads env vars). This works but looks inconsistent. Consider documenting why it's not included in the Promise.all().

🎯 Suggestions

1. Integration Tests

Since this is cross-package functionality (CLI + plugins), consider adding integration tests that:

• Start a build using the plugin with the new API
• Verify correct git info is captured in the build metadata
• Test in simulated CI environments

2. Documentation

The CLAUDE.md should be updated to mention the new services.git.detect() API in the plugin section:

**Plugin Architecture:**
...
Plugins receive access to:
- `services.git.detect()` - Git detection with CI environment support (v0.25.0+)
- `services.testRunner` - Build lifecycle management
- `services.serverManager` - Server control

3. Consider Async PR Number Detection

Future enhancement: Some CI providers might need async operations to detect PR numbers (e.g., calling GitHub API). Consider making detectPullRequestNumber() async now to avoid breaking change later.

🔒 Security

No security concerns. The changes reduce attack surface by removing dynamic imports and standardizing on official APIs.

🎨 Code Style

Follows project conventions well. Consistent with the service-oriented architecture described in CLAUDE.md.

📊 Performance

No performance concerns. Git detection happens once per build, not in hot paths.

Summary

This is a solid improvement that solves a real problem (fragile dynamic imports breaking git detection). The main blocker is missing test coverage for the new public API. Once tests are added and the error handling is clarified, this should be good to merge.

Recommended actions before merge:

1. Add comprehensive unit tests for services.git.detect()
2. Document null-handling behavior or add explicit error handling
3. Add runtime warning when falling back to old behavior
4. Update CLAUDE.md to document the new plugin API

Approval Status: Request changes (tests required)

Vizzly - Visual Test Results

_{feat/plugin-git-api · abbb534f}

Vizzly - Visual Test Results

_{feat/plugin-git-api · bd8371ca}

Vizzly - Visual Test Results

_{feat/plugin-git-api · ce8d33bb}